Skip to content

Remove reindex special case from the progress bar label#697

Merged
maflcko merged 1 commit intobitcoin-core:masterfrom
maflcko:2301-gui-reindex-👖
Feb 7, 2023

Hidden character warning

The head ref may contain hidden characters: "2301-gui-reindex-\ud83d\udc56"
Merged

Remove reindex special case from the progress bar label#697
maflcko merged 1 commit intobitcoin-core:masterfrom
maflcko:2301-gui-reindex-👖

Conversation

@maflcko
Copy link
Contributor

@maflcko maflcko commented Jan 17, 2023

The user knows which option they passed to the program, so it seems overly verbose to offer the user feedback whether or not they passed -reindex. Treat it as DISK, like all other cases that are treated as DISK:

  • -reindex-chainstate
  • -loadblock

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 17, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK john-moffett, hebasto
Stale ACK LarryRuane

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@hebasto hebasto requested a review from ryanofsky January 17, 2023 15:46
@maflcko
Copy link
Contributor Author

maflcko commented Jan 17, 2023

For reference:

  • -reindex-chainstate remains unchanged:
    rcs

  • -loadblock remains unchanged:
    lblk

  • -reindex stage 2 remains unchanged:
    reindex_same_2

  • -reindex stage 1 (master):
    reindex_master

  • -reindex stage 1 (this pull):
    reindex_1_pull

@DrahtBot DrahtBot mentioned this pull request Jan 18, 2023
Merged
@maflcko maflcko added the UI All about "look and feel" label Jan 18, 2023
@maflcko
Copy link
Contributor Author

maflcko commented Jan 28, 2023

So the only user-visible change should be changing the word "Reindex" to "Index"

@hebasto
Copy link
Member

hebasto commented Feb 2, 2023

Concept ACK.

As this PR touches code outside the src/qt directory, i.e., src/interfaces/node.h and src/node/interfaces.cpp, should it be moved to the main repo for a broader reviewing?

@maflcko
Copy link
Contributor Author

maflcko commented Feb 2, 2023

It is only changing an interface that the gui uses, so I think it should be fine

@hebasto
Copy link
Member

hebasto commented Feb 2, 2023

cc @ryanofsky

LarryRuane
LarryRuane approved these changes Feb 4, 2023
View reviewed changes
Copy link
Contributor

@LarryRuane LarryRuane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code-review, tested ACK fa20044

I ran src/qt/bitcoin-qt -noconnect -signet -reindex with and without the PR, and the status message (lower-left corner) for stage 1 was as expected. I didn't try reindex-chainstate, loadblock, or reindex stage 2.

src/qt/clientmodel.cpp
Comment on lines +151 to +152
if (m_node.isLoadingBlocks()) return BlockSource::DISK;
if (getNumConnections() > 0) return BlockSource::NETWORK;
Copy link
Contributor

@LarryRuane LarryRuane Feb 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, somehow else following return (or break, continue) doesn't seem right.

john-moffett
john-moffett approved these changes Feb 6, 2023
View reviewed changes
Copy link
Contributor

@john-moffett john-moffett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa20044

Nit: Maybe remove the one remaining unnecessary enum use for BlockSource?

gui/src/qt/bitcoingui.cpp

Line 1074 in fa20044

enum BlockSource blockSource = clientModel->getBlockSource();

@maflcko
Copy link
Contributor Author

maflcko commented Feb 7, 2023

Thanks, removed and rebased. Should be trivial to re-ACK with git range-diff

@john-moffett
Copy link
Contributor

john-moffett commented Feb 7, 2023

Re-ACK faff2ba

hebasto
hebasto approved these changes Feb 7, 2023
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK faff2ba, I have reviewed the code and it looks OK, I agree it can be merged.

@maflcko maflcko merged commit 1bcabe6 into bitcoin-core:master Feb 7, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 7, 2023
Merge bitcoin-core/gui#697: Remove reindex special case from the prog…
50203a8
@LarryRuane
Copy link
Contributor

LarryRuane commented Feb 8, 2023

post-merge re-ACK

@maflcko maflcko deleted the 2301-gui-reindex-👖 branch February 8, 2023 10:57
@ryanofsky ryanofsky mentioned this pull request Feb 13, 2023
Draft
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Feb 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

@ryanofsky ryanofsky Awaiting requested review from ryanofsky

+2 more reviewers

@LarryRuane LarryRuane LarryRuane approved these changes

@john-moffett john-moffett john-moffett approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

UI All about "look and feel"

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maflcko @DrahtBot @hebasto @john-moffett @LarryRuane

Comments